refactor(l1): deduplicate the account-update loop in Store - #7184
refactor(l1): deduplicate the account-update loop in Store#7184ilitteri wants to merge 4 commits into
Store#7184Conversation
apply_account_updates_from_trie_batch and apply_account_updates_from_trie_with_witness ran the same per-update algorithm, copy-pasted. Extract it into a private apply_account_updates_from_trie_inner and turn both public functions into thin wrappers with unchanged signatures. The variants' only real differences stay behind an Option<&mut StorageTries>: None opens a fresh storage trie per account (batch); Some caches tries in the map keyed by unhashed address, reusing the logged trie on hit and wrapping newly opened ones in TrieLogger::open_trie so witness recording spans updates. Hashed addresses unify on hash_address_fixed, the same keccak digest the witness copy re-wrapped with H256::from_slice.
|
🤖 Codex Code Review
Other than that, the refactor looks mechanically equivalent. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewI have enough to complete the review based on careful manual trace of the diff (the two original implementations vs. the unified inner function), confirming key equivalences ( Review: PR 7184 — deduplicate account-update loop in
|
Lines of code reportTotal lines added: Detailed view |
| )?)) | ||
| } | ||
|
|
||
| pub fn apply_account_updates_from_trie_batch<'a>( |
There was a problem hiding this comment.
I think this function can be inlined. I don't see any uses apart from the one in apply_account_updates_batch.
| account_state.balance = info.balance; | ||
|
|
||
| account_state.code_hash = info.code_hash; | ||
|
|
There was a problem hiding this comment.
This isn't a good way to reduce lines of code.
| let account_updates_list = AccountUpdatesList { | ||
| Ok(AccountUpdatesList { |
There was a problem hiding this comment.
This is also not a good way to reduce lines of code
| state_trie.insert( | ||
| hashed_address.as_bytes().to_vec(), | ||
| account_state.encode_to_vec(), | ||
| )?; |
There was a problem hiding this comment.
This would be a good way to reduce lines of code:
| state_trie.insert( | |
| hashed_address.as_bytes().to_vec(), | |
| account_state.encode_to_vec(), | |
| )?; | |
| let hashed_address_vec = hashed_address.as_bytes().to_vec(); | |
| state_trie.insert(hashed_address_vec, account_state.encode_to_vec())?; |
…cal blocks is deliberate readability structure and never counted as lines of code — the reduction here is the deduplicated logic, not whitespace.
…elong The previous commit's blank lines were inserted mechanically rather than at logical-block boundaries: one landed between a doc comment and the item it documents, which clippy rejects as empty_line_after_doc_comments and which failed `cargo clippy -- -D warnings`; others split a `let` binding from its right-hand side, split a call's argument list, and sat immediately after an opening brace or before a closing one. Two more landed in setup_genesis_state_trie, a function this change does not otherwise touch. Restore the spacing from the source the shared loop was actually extracted from: apply_account_updates_from_trie_with_witness. Its body is the one that survives in apply_account_updates_from_trie_inner, so its blank lines go back exactly where they were and nowhere else. The deleted batch copy had no such spacing to preserve, and setup_genesis_state_trie returns to its original form. Blank lines are not lines of code either way; the reduction remains the deduplicated logic.
…te loop The extraction landed the per-variant switch as a match wrapping a match wrapping a block, with the open_storage_trie call spelled out in full in both arms. Collapsing it — one closure for the shared fresh-open call, `&mut ....1` instead of re-destructuring the cached (witness, trie) pair, and Option::insert instead of a deferred-init local — takes the acquisition from 24 code lines to 13 and drops a nesting level, with both variants' semantics untouched: Occupied still reuses the cached trie without consulting account_state.storage_root, Vacant still wraps the new trie in TrieLogger::open_trie and inserts it into the map keyed by the unhashed address, and the storage-tries-less variant still opens a fresh trie per account at that account's current storage root. The closure keeps the open lazy, so an Occupied hit still never opens a trie. With this, the shared loop body is byte-identical to the batch loop it replaced everywhere except the acquisition block, and the whole deduplication is 126 -> 91 code lines across the two entry points.
2 participants
Motivation
Store::apply_account_updates_from_trie_batchandStore::apply_account_updates_from_trie_with_witnesswere two hand-maintained copies of the same per-update algorithm: remove the account ifupdate.removed; decode-or-default theAccountState; resetstorage_rootonremoved_storage; copy nonce/balance/code_hash and collectcode_updates; applyadded_storageto a storage trie andcollect_changes_since_last_hash; insert the account; finally collect state changes into anAccountUpdatesList. Any future change to the update semantics had to be made twice, and the copies had already drifted cosmetically (hashed-address type, blank-line style), which obscured that the only real difference is how the storage trie is acquired.Description
Extracts the loop into a private
apply_account_updates_from_trie_inner(&self, state_trie, account_updates, storage_tries: Option<&mut StorageTries>)and turns both public functions into thin wrappers with their exact original signatures.crates/storage/store.rsis the only file touched:+46/−79, a net −35 lines of code.Line accounting (code lines only — blank and comment-only lines counted as zero, the way
tokeicounts):apply_account_updates_from_trie_batchapply_account_updates_from_trie_with_witnessapply_account_updates_from_trie_innerOf that, the duplicated loop bodies go 113 → 64 code lines; the three signatures plus their delegating calls cost 27 where the two originals cost 13. Blank lines were not touched to move the number: the diff's blank-line count is +1 and its comment count +1, and the surviving loop keeps the blank-line structure of
apply_account_updates_from_trie_with_witness, the function it was extracted from.The merged body is byte-identical (modulo blank lines) to main's batch loop body everywhere except the storage-trie acquisition block, and identical to main's witness body except for that block, the hashed-address type, and the return shape. Both diffs are small enough to read line by line, which is how each invariant below was checked.
The two variants' only genuine differences are preserved verbatim behind the
Option:None(batch) opens a fresh trie per account viaopen_storage_trieat that account's currentstorage_root, exactly as before.Some(map)(witness) uses the entry API on the map keyed by unhashedupdate.address:Occupiedreuses the cached(TrieWitness, Trie)— the cached trie's in-memory state intentionally wins overaccount_state.storage_root, including ignoring anEMPTY_TRIE_HASHreset fromremoved_storage— andVacantopens the trie after theremoved_storagereset, wraps it inTrieLogger::open_trie, and inserts it into the map so witness nodes for storage tries first touched during update application are recorded; the mutated map is returned to the caller as before.open_storage_triecall lives in a closure so it stays lazy: anOccupiedhit never opens a trie, just as it never did before.hash_address_fixed(H256). The witness copy usedhash_address(Vec<u8>) and converted back withH256::from_slice; both are the same keccak digest of the address, so trie keys andstorage_updateskeys are byte-identical. This also drops theH256::from_slicelength-panic path.state_rootpassed toopen_storage_trieremains the single pre-loophash_no_commitsnapshot, not recomputed per iteration.Option<&mut StorageTries>is reborrowed per iteration withas_deref_mut(), and the batch arm parks its fresh trie in a per-iterationOptionlocal so both arms unify as&mut Trie.Public API, RLP encoding, DB schema, and
STORE_SCHEMA_VERSIONare untouched. No tests were deleted or modified.If this change were wrong, one of these would have to be true:
hash_addressandhash_address_fixedwould have to produce different bytes for some address — they are bothkeccak(address.to_fixed_bytes())(store.rs), one returningVec<u8>, the otherH256.removed_storageroot — it cannot: theOccupiedarm never consultedaccount_state.storage_rootbefore this change either, and the closure that would do the opening is not called on that arm.StorageTriesby hashed address — callers inblockchain.rspopulate and drain it keyed by unhashedAddress, which this change keeps.How to test
Results on this branch:
cargo fmt --all --check— clean.cargo clippy -- -D warnings— clean, exit 0. This is the exact command the L1 Lint job runs, and it is the gate that previously failed on this branch withclippy::empty_line_after_doc_comments.cargo clippy --workspace --all-targets -- -D warnings— clean.cargo test -p ethrex-storage— 91 passed, 0 failed (plus doctests: 1 passed, 1 ignored).cargo test -p ethrex-blockchain— 58 passed, 0 failed.cargo test -p ethrex-test --test ethrex_tests batch— 30 passed, 0 failed. Includesbatch_selfdestruct_created_account_no_spurious_state, which exercises exactly the default-AccountState+removed_storage+ state-root-parity semantics of this loop end to end, plusaccount_batch_parity_in_memoryandstorage_batch_parity_in_memory.vectors_zkevmbundle — so that invariant was checked by reading theVacantarm against the original line by line: it still wraps inTrieLogger::open_trie, still inserts into the map, and the wrapper still returns the mutated map.Checklist
Storeschema change, soSTORE_SCHEMA_VERSION(crates/storage/lib.rs) is untouched — this is a private helper extraction; public API, RLP encoding and DB layout are identical, so no re-sync is required.